-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stun Expiration #613
base: master
Are you sure you want to change the base?
Stun Expiration #613
Conversation
- There's an extra signal sent on wrapping around in initiative that breaks the logic for when to tick because both new and old initiative numbers are 0 - The logic is still faulty if there are duplicate initiative numbers, but this at least replicates what seems to be the intended original behavior.
- There seems to have been a mistake translating it from the original, which always caused a 0->0 update message, rather than an N->0 update message on wrapping around (and thus no N->0 message was sent before the 0->MAX message, causing some intervals to be skipped). - There probably shouldn't be an N->0 message when N is already <=0 due to actual rolled initiatives being that low.
@@ -120,8 +120,15 @@ void TurnBasedSys::InitiativeListNextActor() | |||
dispatch.DispatcherProcessor(dispatcher, dispTypeInitiative, 0, 0); | |||
} | |||
} | |||
actorInitiative = nextInitiativeIdx = 0; | |||
InitiativeRefresh(actorInitiative, 0); | |||
nextInitiativeIdx = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I must confess I don't fully understand what was going on here, and in general these sorts of changes have to be done very carefully.
Also, I don't know if it's an issue, but have you considered/checked what happens with concurrent turns enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line was already changed from the original code. Can you explain why?
In the original code, it sets nextInitiativeIdx
to 0, then calls InitiativeRefresh
, then it sets actorInitiative
to 0. The effect this has is sending a signal that counts from the current initiative to 0, so that after this when you count from 0 to the max initiative, no numbers have been skipped. In TemplePlus, instead there is a signal sent with two 0s, which was actually the reason that Stunned was broken.
However, an actual initiative can be 0 or negative, which is why I added the check after these lines, because they will cause signals that count from, say, -1 to 0, before wrapping around, which have the same bad effects as the 0->0 signal, but are harder to trigger.
The way Stunned uses these events, it shouldn't even be necessary to actually count to/past 0 before wrapping, but I don't know what else listens to these events.
I'm not sure how concurrent turns interacts with this exactly, but this is just fixing the numbers in the initiative change broadcasts, so it seems unlikely that it would be broken in ways that it wasn't already (like, counting down too far while an earlier turn is still happening, and missing some bonuses).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I should also mention, one of the reasons I fixed this is that there could otherwise be situations where someone is stunned permanently, because the stunned condition is set to tick during an initiative number that is skipped. You could do this by stunning someone as the lowest (but positive) initiative, then delaying your turn so that you have a higher initiative in the next round. Then your previous initiative number is skipped for the rest of combat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, looks like a simple mistake. I only replaced that function for debugging apparently (the irony :P)
Looking at the condition list, Stunned is the only one that reacts to S_Initiative_Update.
- Clarify relation to initiative signals, and generalize the avoidance of 'degenerate' messages, although they shouldn't occur anymore. - Made the intervals mentioned in the comments no longer backwards.
So, I've been thinking a while about how to make the ticking more precise, but I don't have any ideas that won't be a lot more fiddly and complicated in various ways. The logic that's in there right now is only a problem if you're stunning someone with the same initiative number, or if a lot of people have that initiative. That isn't a likely occurrence unless you go wild delaying turns, so I'm inclined to leave it. The other option would be to completely replace the ticking with the usual ticker that runs on the target's turn. But I'm not sure I have the right information for that (e.g. the address of the |
Well the way the game resolves same initiative is through sub initiative. If there isn't a spare arg you could pack it with bitmasking. |
You're pretty sure that this is the only condition that actually listens to the signal currently? If I made the initiative tick on every transition with some packing scheme with the subinitiative, that might work. And some logic could be used to try to catch cases where people saved in the middle of combat while someone was stunned. My concern with that was just what other stuff it would affect. |
Yeah, pretty sure (are you familiar with the conditions list on the wiki? https://github.com/GrognardsFromHell/DllDocumentation/wiki/Conditions-(List) ) |
Both args are already taken unfortunately. It sends the old and the new initiative. And I think that's necessary, because even if every turn sends a signal, delayed turns might end up with the exact number being skipped. |
Ah ok. |
It's a D20 signal, though, so it'd probably be better not to add fields to that event object, right? I don't think it's a big deal to pack the values. You'd have to be doing something seriously degenerate to have an initiative and subinitiative that don't fit in 16 bits each. I think the game uses something like 100*dex for initializing the subinitiative, so you'd need a dexterity score of 656 for it to be a problem. |
That's true, I just hate bit packing, always feels so hacky 😛 However, the best approach is probably to make the signal arg a pointer to a suitable struct containing all the relevant info. You could even add time stamps and whatnot. |
So, I've been studying this a bit, and there's an additional complication. The stuff that's been pulled into Temple+ just calls an original function with the relevant initiatives. That function is actually responsible for some BeginRound stuff too, for instance, so it's more complicated than just sending the initiative step signal. However, initiative stepping in combat also isn't the only place that function is called. When out of combat, the function that handles world time also tries to count initiatives from 25 to 0 every 6 seconds. And that doesn't appear to be replicated, either. Without replacing that, the old sort of signal would still be sent out of combat. So, obviously, that's a lot more complicated than just sending one different signal that is only listened to by one condition. |
Starting on fixing the removal logic of stun (the stunning fist version; possibly some monster effects).
This commit should restore the intention of the original logic, which tries to actually match the way the P&P rules say you should count the rounds. It was being fouled up because there's an extra initiative signal sent when the turn wraps around with arguments it didn't account for.
However, the logic is still slightly faulty if the attacker and target have the same initiative number. If the tie is broken with the target going first, they won't actually lose a turn. I'm still trying to figure out how best to solve that, because I don't think enough information is actually stored for the precisely correct behavior.